Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(PlanarLayer): Fix delete new extent from globalExtentTMS const #2279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ketourneau
Copy link
Contributor

@ketourneau ketourneau commented Feb 23, 2024

Proposed fix for #2244

Motivation and Context

When PlanarLayer is deleted, it doesn't remove extent from globalExtentTMS const.
It's why we got some trouble when we try to reload itowns with new extent. It keep previous extent added to globalExtentTMS.

I don't know if it's better to reset globalExtentTMS when view is disposed or remove extent from globalExtentTMS if PlanarLayer is deleted.

I think my fix may cause a problem if two PlanarLayer use the same extent and one was deleted.

What do you think ?

@jailln jailln self-assigned this Feb 26, 2024
@jailln
Copy link
Contributor

jailln commented Feb 26, 2024

Thanks for finding and correcting this bug 🙂

It actually corrects the issue but I think that we need to re-think the globalExtentTMS implementation/usage as it doesn't allow to have multiple views with the same CRS and different extents at the same time.

It seems that globalExtentTMS is used for two things:

  • actually storing the "global extent" (or default extent) or a CRS (like EPSG:3857 global extent)
  • storing the view extent in a global variable and use it to update layers (e.g. in LayeredMaterialNodeProcessing, LabelLayer); in contexts where we already have access to the view and therefore to its extent (with context.view).

I propose that we limit the usage of globalExtentTMS to the first usecase and therefore:

  • don't set new values in PlanarLayer constructor.
  • Either force to provide an extent for TMSSource or set TMSSource.extent in _preprocessLayer instead than in TMSSource constructor. (I like the second option more)

WDYT @ketourneau @Desplandis ?

Disclaimer: I tried to explain my thoughts as clear as possible but we may discuss it in a voice call if needed 😁

jailln
jailln previously approved these changes Feb 26, 2024
src/Core/Prefab/Planar/PlanarLayer.js Outdated Show resolved Hide resolved
@ketourneau
Copy link
Contributor Author

I agree with you.
I also wondered whether globalExtentTMS should be used for global extent (like EPSG:3857).

@jailln
Copy link
Contributor

jailln commented Feb 27, 2024

I also wondered whether globalExtentTMS should be used for global extent (like EPSG:3857).

You mean we should remove it totally?

@ketourneau
Copy link
Contributor Author

No, I just wanted to say that I agree with your suggestion.
I'm waiting to see what you decide.

@Desplandis
Copy link
Contributor

Desplandis commented Mar 14, 2024

@jailln @ketourneau : According to @mgermerie, the root cause of this issue seems to be that tiles of the terrain (as created by the view) does not match that of an actual TMS. He detailed different solutions for this issue in the #2290 proposal, which could fix other related issues in iTowns.

@ketourneau
Copy link
Contributor Author

@Desplandis @mgermerie We also have the same error with cog example when we try to dispose view and load another cog.

If you Load 1 band sample and then Load RGB sample with this commit : bloc-in-bloc@833fc68 you will got this result :
Capture d’écran 2024-03-15 à 08 32 33

It's the same error, when we call view.dispose(true) it doesn't remove extent from globalExtentTMS const.
So if we reload view with another extent we got problem.

@Desplandis
Copy link
Contributor

@ketourneau Yeah I think this again a mix between the issue described in #2290 and cache coherency...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants